Skip to content
This repository was archived by the owner on Jun 2, 2020. It is now read-only.

Snaji/create persipubsub - #1

Open
slimeth wants to merge 35 commits into
masterfrom
snaji/create-persipubsub
Open

Snaji/create persipubsub#1
slimeth wants to merge 35 commits into
masterfrom
snaji/create-persipubsub

Conversation

@slimeth

@slimeth slimeth commented Jan 29, 2019

Copy link
Copy Markdown
Contributor

No description provided.

@slimeth
slimeth requested a review from radomsak January 29, 2019 09:17

@radomsak radomsak left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Selim

I only reviewed the readme file. I'll work on the rest in the afternoon

Adam

Comment thread README.rst Outdated
Comment thread README.rst Outdated
Comment thread README.rst Outdated
Comment thread README.rst Outdated
Comment thread README.rst Outdated
Comment thread README.rst Outdated
Comment thread README.rst
Comment thread README.rst Outdated
Comment thread README.rst Outdated
Comment thread README.rst Outdated
@slimeth
slimeth force-pushed the snaji/create-persipubsub branch 2 times, most recently from 73e0c51 to 9b9a710 Compare February 5, 2019 15:21
@slimeth
slimeth force-pushed the snaji/create-persipubsub branch from 9b9a710 to e41f3cd Compare February 5, 2019 15:24

@radomsak radomsak left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review part 1

  1. You need to decide whether publisher sends messages to the subscriber, or maybe it only writes messages to the queue and then subscriber read them. Once you make up your mind you should make sure that all the comments are consistent (some of my suggestions might not apply then)

  2. Please explain what exactly happens in get_queue_data function offline. Maybe there is a way to not have this function

Comment thread persipubsub/__init__.py Outdated
Comment thread persipubsub/__init__.py Outdated
Comment thread persipubsub/__init__.py Outdated
Comment thread persipubsub/environment.py Outdated
Comment thread persipubsub/environment.py
Comment thread persipubsub/__init__.py Outdated
Comment thread persipubsub/__init__.py Outdated
Comment thread persipubsub/__init__.py Outdated
Comment thread persipubsub/control.py Outdated
Comment thread persipubsub/__init__.py Outdated

@radomsak radomsak left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Selim,

The code looks really good! There are only minor comments and changes. You did great work.

I reviewed up to tests. More comes tomorrow afternoon

Two comments for now

  1. You need to decide whether publisher sends messages to the subscriber, or maybe it only writes messages to the queue and then subscriber read them. Once you make up your mind you should make sure that all the comments are consistent (some of my suggestions might not apply then)
  2. Please explain what exactly happens in get_queue_data function offline. Maybe there is a way to not have this function

Adam

Comment thread persipubsub/control.py Outdated
Comment thread persipubsub/control.py Outdated
Comment thread persipubsub/control.py Outdated
Comment thread persipubsub/control.py Outdated
Comment thread persipubsub/control.py Outdated
Comment thread persipubsub/queue.py Outdated
Comment thread persipubsub/queue.py Outdated
Comment thread persipubsub/queue.py Outdated
Comment thread persipubsub/queue.py
Comment thread persipubsub/queue.py

@radomsak radomsak left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Selim,
I looked at the unit tests. they are very nicely structured and clearly written. I think there is no need to do any big changes. Please see minor 2 comments below

Adam

Comment thread tests/test_live.py
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants